Drop the pynufft legs from the NUFFT parity script - #261
Merged
Conversation
`TransformerNUFFTPyNUFFT` no longer exists, so `nufft.py` could not run at all. The script's original purpose — proving nufftax matched pynufft closely enough to replace it — is served: that swap shipped, and pynufft has now been removed from PyAutoArray entirely. What remains is the standing correctness check, with `TransformerDFT` (a direct, unapproximated Fourier transform) as the sole reference: - tests (a) and (b) drop the pynufft leg and add an explicit library-vs-DFT assertion, so the shipped transformer is checked against the exact reference rather than only against the local nufftax recipe. - test (c) used pynufft as its mapping-matrix reference; it now uses `TransformerDFT.transform_mapping_matrix`. The tolerance tightens from 1e-1 (pynufft's gridding precision) to 1e-9, since the new reference is exact. - test (d) needed no functional change; only its pynufft prose is gone. - the residuals plot drops to two panels — the shipped transformer and the local recipe agree bit-for-bit, so the third panel was an empty log axis. The equality is still asserted. - `import warnings` removed; every `catch_warnings` block existed to silence pynufft. The full-resolution requirement is re-justified rather than dropped. It existed because pynufft's gridding error scaled with N, so the tolerances only held at 256x256; nufftax is accurate at any N (3.0e-14 vs 1.7e-14 relative at 256x256 and 16x16). The 256x256 guard stays because it is the production geometry the JAX likelihood scripts use, and it is now the only thing that catches a silently-capped run. In `rectangular_sparse.py`, the recorded reason for avoiding `apply_sparse_operator` with `TransformerNUFFT` was specific to pynufft's kernel-deconvolved adjoint scale. That backend is gone and whether the incompatibility still holds has not been re-verified, so the dead attribution is removed without asserting a replacement claim; the path does not depend on the answer. Verified: `python scripts/interferometer/nufft.py` passes all four tests (max relative residual 3.0e-14 vs DFT at 256x256) with no warnings.
3 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Workspace-test follow-up to PyAutoLabs/PyAutoArray#475 (merged), which removed
TransformerNUFFTPyNUFFT.scripts/interferometer/nufft.pyconstructed that class directly, so it could not run at all after the removal. Its original purpose — proving nufftax matched pynufft closely enough to replace it — has been served: that swap shipped, and pynufft is now gone from PyAutoArray entirely.What remains is the standing correctness check, with
TransformerDFT(a direct, unapproximated Fourier transform) as the sole reference:TransformerNUFFTis now checked against the exact reference rather than only against the local nufftax recipe.TransformerDFT.transform_mapping_matrix. The tolerance tightens from 1e-1 to 1e-9 — the old one was pynufft's gridding precision, the new reference is exact.import warningsremoved — everycatch_warningsblock existed to silence pynufft.The full-resolution requirement is re-justified rather than dropped. It existed because pynufft's gridding error scaled with N, so the tolerances only held at 256x256. nufftax is accurate at any N (measured 3.0e-14 vs 1.7e-14 relative at 256x256 and 16x16), so the
ENV: full_datasetsdeclaration and the geometry guard stay for a different reason: 256x256 / 0.1" is the production geometry the JAX likelihood scripts use, and the guard is now the only thing that catches a silentlyPYAUTO_SMALL_DATASETS-capped run.In
scripts/interferometer/jax_likelihood/rectangular_sparse.py, the recorded reason for avoidingapply_sparse_operatorwithTransformerNUFFTwas specific to pynufft's kernel-deconvolved adjoint scale. That backend is gone and I have not re-verified whether the incompatibility still holds against the nufftax adjoint, so the dead attribution is removed without asserting a replacement claim. That path deliberately does not depend on the answer. Worth a look separately.Scripts Changed
scripts/interferometer/nufft.py— the substantive rewrite (73 insertions, 105 deletions)scripts/interferometer/jax_likelihood/rectangular_sparse.py— comment onlyscripts/CLAUDE.md— the script-catalogue entry, which described the file as a pynufft-vs-nufftax parity testTest Plan
python scripts/interferometer/nufft.pyruns end to end and passes all four tests, no warnings:max |Δ| nufftax - DFT= 1.5e-13,library - DFT= 1.5e-13scripts/interferometer/images/nufft_residuals.pngGenerated by Claude Code